feat: Add external plugins support - DO NOT MERGE #183
Conversation
Signed-off-by: Maya Barnea <[email protected]>
There was a problem hiding this comment.
I did not review beyond the first or second file.
IMO, this PR might be too big to review in one go and could benefit from breaking up (e.g.,):
- design doc/proposal (which should be approved separately)
- API definitions (e.g., JSON between client and server)
- sample server (e.g., python) and client, for a fixed function.
- improved client (e.g., configuration of http.Client for timeout handling, etc.)
There is no need to support all plugin types at once. Furthermore, since configuration via file is forthcoming, I would delay writing configuration code until it is merged. Otherwise it's throwaway code.
| @@ -14,6 +14,7 @@ require ( | |||
| github.com/prometheus/client_golang v1.22.0 | |||
| github.com/redis/go-redis/v9 v9.7.3 | |||
| github.com/stretchr/testify v1.10.0 | |||
| github.com/valyala/fasthttp v1.62.0 | |||
There was a problem hiding this comment.
q: what's the benefit of using a non standard HTTP implementation? If performance, then we should first prove that HTTP handling is a bottleneck.
| @@ -52,15 +54,49 @@ const ( | |||
|
|
|||
| prefixScorerBlockSizeEnvKey = "PREFIX_SCORER_BLOCK_SIZE" | |||
| prefixScorerBlockSizeDefault = 256 | |||
|
|
|||
There was a problem hiding this comment.
At this point the external process plugin is for demo only, so I would refrain from introducing and using all these environment variables (which would be eliminated by the config changes anyway).
There was a problem hiding this comment.
For a demo, you can just enable (or not) a fixed type plugins (e.g., filter or scorer) at a well known address (e.g., localhost:8000/8088 and run their container(s) in the same Pod)
| @@ -22,6 +22,7 @@ import ( | |||
| logutil "sigs.k8s.io/gateway-api-inference-extension/pkg/epp/util/logging" | |||
|
|
|||
| "github.com/llm-d/llm-d-inference-scheduler/pkg/config" | |||
| externalhttp "github.com/llm-d/llm-d-inference-scheduler/pkg/scheduling/plugins/external/http" | |||
mayabar
changed the title
mayabar
changed the title
elevran
added
the
do-not-merge/hold
|
@elevran what’s the intention about this PR? |
|
It is not intended to be merged at this time and was created to share ideas with community and to solicit feedback (the concept of out of process plugins was presented at a community meeting and had corresponding code for those interested). |
|
one option would be to create a branch called |
|
Agree. |
Add initial version of server side support for external HTTP plugins.
Add sample filter plugin implemented in python
Fixes: #163
Fixes: #164
Fixes: #184